Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(styles): isolated point style overrides #2278

Merged
merged 13 commits into from
Jan 26, 2024
Merged

fix(styles): isolated point style overrides #2278

merged 13 commits into from
Jan 26, 2024

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Dec 9, 2023 •
edited
Loading

Summary

This change applies a hierarchy to the (AreaSeriesStyles|LineSeriesStyles).isolatedPoint styles. This is to prevent changing values in one to override the other. If the consumer applies a style to the points they would expect the style to transfer to all points.

The hierarchy would be (in order of highest priority):

  • (AreaSeriesStyles|LineSeriesStyles).isolatedPoint
  • (AreaSeriesStyles|LineSeriesStyles).point
    • This was the missing piece in the current logic
    • Skips visible override
  • Theme.(areaSeriesStyle|lineSeriesStyle).isolatedPoint

Details

I don't think the onus of this styling should be on the chart consumer to always supply a backup style to isolatedPoint whenever they are setting the point style.

Issues

fix #2277

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with light and dark themes

@nickofthyme nickofthyme requested a review from markov00 December 9, 2023 00:34
@nickofthyme nickofthyme marked this pull request as ready for review January 9, 2024 20:55
@nickofthyme nickofthyme added :styling Styling related issue :xy Bar/Line/Area chart related labels Jan 9, 2024
markov00
markov00 reviewed Jan 11, 2024
View reviewed changes
e2e/package.json Outdated Show resolved Hide resolved
@nickofthyme nickofthyme requested a review from markov00 January 11, 2024 23:11
@nickofthyme
Copy link
Collaborator Author

@markov00 could you take another look at this?

@nickofthyme
Copy link
Collaborator Author

Comment from #2277 (comment)

There is one more issue related to that:
isolated points in Stacked areas are not shown if the consumer legitimately sets the areaPointStyle visibility to false. This with our default theme (EUI) isolated point on these time of charts are interactive but not visible:

Screenshot 2023-12-18 at 15 56 03

At this point, an isolated point should inherit the style of a point except for the visibility that should always be true, except if explicitly disabled in the isolated point style.

markov00
markov00 approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine for me, thanks!

@nickofthyme nickofthyme merged commit 3fb1df2 into elastic:main Jan 26, 2024
13 checks passed
@nickofthyme nickofthyme deleted the isolated-point-style branch January 26, 2024 22:42
nickofthyme pushed a commit that referenced this pull request Jan 29, 2024
@semantic-release-bot
chore(release): 63.1.0 [skip ci]
694e80f 
# [63.1.0](v63.0.0...v63.1.0) (2024-01-29)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^92.1.1 ([#2315](#2315)) ([f4e4fae](f4e4fae))
* **deps:** update dependency @playwright/test to ^1.41.1 ([#2316](#2316)) ([e2ab527](e2ab527))
* **styles:** isolated point style overrides ([#2278](#2278)) ([3fb1df2](3fb1df2))

### Features

* **metric:** custom slot to render contents in gap ([#2303](#2303)) ([3256c8c](3256c8c))

### Performance Improvements

* **tooltip:** improve placement logic ([#2310](#2310)) ([cac5f49](cac5f49))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
:styling Styling related issue :xy Bar/Line/Area chart related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Isolated point styles override user defined point styles
2 participants
@nickofthyme @markov00